REV Code Review Report

Status: PASS

Scope reviewed: #267 at 7fcda8ee95bec7a3de5f8142e1dc96afaba55bbc.

Checks:

• Diff is narrow: get_queue_info() / get_queue_info(text) privilege fix plus one regression test.
• SECURITY DEFINER is paired with SET search_path = pgque, pg_catalog in generated install SQL.
• build/transform.sh is the source-of-truth patch point and has a guard requiring exactly two overloads to be promoted.
• Regression covers the actual granted role (pgque_reader) and both overloads after exercising the seq_getval() path.
• CI is green for the PR’s full matrix.

No blockers found. This is ready from REV/code-review perspective.

samorev Code Review Report

• PR: fix: make get_queue_info reader-callable #267 - fix: make get_queue_info reader-callable
• Author: @NikolayS
• AI-Assisted: Unknown

No issues found. Reviewed for security, bugs, tests, guidelines, and documentation.

Result: PASSED

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/PgQue
number=267
target=github:NikolayS/PgQue#267
state=OPEN
draft=false
diff_lines=157
diff_added=89
diff_removed=4
diff_bytes=5587
comments_count=1
commits_count=2
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)

Post-merge code review — thorough pass

Reviewed the full diff against CLAUDE.md rules (SECURITY DEFINER contract, generated-file consistency, red/green TDD, SQL style) and verified the key claims empirically on PostgreSQL 18 in Docker.

Verdict: no blocking findings. The fix is correct, minimal, properly tested, and reproducible. Details below, including the security angle I checked hardest.

Correctness — clean

• get_queue_info(text) calls pgque.seq_getval(_queue_event_seq); _queue_event_seq is read from the pgque.queue.queue_event_seq column, not from a caller argument. No injection surface via the function's text parameter.
• Both overloads land as SECURITY DEFINER with SET search_path = pgque, pg_catalog — verified in the live catalog (prosecdef = t, proconfig = {search_path=pgque, pg_catalog}), satisfying the CLAUDE.md mandate.
• No new information exposure: the zero-arg overload returning all-queue metadata was already reader-granted and intended for readers; DEFINER only fixes the seq_getval execute failure, it doesn't widen the row set.

Security — PUBLIC-execute trap checked and NOT present

The one real risk when flipping a function to SECURITY DEFINER is Postgres's default PUBLIC EXECUTE grant: DEFINER + PUBLIC would let any role run it with owner privileges. Verified this is handled:

• sql/pgque.sql:4824 does revoke execute on all functions in schema pgque from public.
• Live ACL on both overloads: {postgres=X/postgres, pgque_reader=X/postgres, pgque_admin=X/postgres} — PUBLIC absent.
• Empirical: a fresh unprivileged role (schema usage only, not in pgque_reader) calling pgque.get_queue_info() gets ERROR: permission denied for function get_queue_info. No leak.

Audit claim ("get_queue_info was the only one") — confirmed

Enumerated every seq_getval call site in the install:

• get_queue_info(text) — the fix.
• inside ticker() — writer/admin path, not a reader monitoring function.
• inside seq_setval() — admin helper, not reader-granted.

Sibling reader-granted monitoring functions get_consumer_info / get_batch_info are already SECURITY DEFINER (verified). next_batch_info is SECURITY INVOKER and reader-granted but reads only tables the reader can already select, and calls no admin-only helper — no trap. The audit in the PR body holds.

Consistency — reproducible build

• Checked out the merge commit, ran bash build/transform.sh fresh: empty diff. sql/pgque.sql and sql/pgque-tle.sql regenerate exactly as committed; both files carry the change.
• The transform patch includes a fail-closed guard (defcount -ne 2 → exit 1), so if upstream PgQ ever restructures these overloads the build breaks loudly rather than silently under-patching. Good defensive touch.

Tests — genuine red/green

Ran tests/test_get_queue_info_reader.sql both ways on PG 18:

• Red (pre-fix SQL, git show <parent>:sql/pgque.sql): fails with the exact ERROR: permission denied for function seq_getval / CONTEXT: ... get_queue_info(text) line 58.
• Green (merged SQL): both overloads pass under set role pgque_reader, ev_new populated.
• Registered in tests/run_all.sql; commit order is test-first then fix. TDD honored.

Nits (non-blocking, no action needed)

• SECURITY DEFINER means these now run as the function owner (a superuser on a typical install). Safe here — static body, no dynamic SQL, pinned search_path — and it mirrors the existing get_consumer_info / get_batch_info pattern. Noted only for completeness.
• Test-file header uses -- for a multi-line block; CLAUDE.md prefers /* */ for 2+ lines, but this matches the established convention in sibling tests (e.g. test_e2e_role_split.sql), so it's consistent with the repo, not a defect.

Reviewed by an independent post-merge pass (build reproduced + tests run red/green + ACLs checked on PostgreSQL 18).

samorev Code Review Report

• PR: fix: make get_queue_info reader-callable #267 - fix: make get_queue_info reader-callable
• Author: @NikolayS
• AI-Assisted: Yes

This report supersedes the earlier samorev review --fetch comment on this PR. That comment was a CI/draft gate check only and performed no code analysis (it hardcoded zeros in every area). The results below come from a genuine per-area review of the diff, with the key claims verified empirically on PostgreSQL 18.

No blocking issues. Reviewed for security, bugs, tests, guidelines, and documentation. Two candidates were examined and filtered as non-issues (see below).

Result: PASSED

What was verified

• Security definer contract: both get_queue_info overloads land as security definer with set search_path = pgque, pg_catalog (live catalog: prosecdef = t, proconfig = {search_path=pgque, pg_catalog}).
• No PUBLIC EXECUTE trap: sql/pgque.sql does revoke execute on all functions in schema pgque from public; live ACL on both overloads has no PUBLIC entry. An unprivileged role (schema usage only) calling pgque.get_queue_info() gets permission denied for function — no privilege leak from the definer flip.
• No injection surface: the text parameter is a queue name; the sequence read is seq_getval(_queue_event_seq) where _queue_event_seq comes from the pgque.queue row, not the caller argument.
• Audit ("only reader function affected") holds: every seq_getval call site enumerated; sibling reader functions get_consumer_info/get_batch_info are already security definer, next_batch_info reads only reader-visible tables.
• Red/green test: tests/test_get_queue_info_reader.sql fails on the pre-fix SQL (permission denied for function seq_getval at get_queue_info(text)) and passes on the fix under set role pgque_reader; registered in tests/run_all.sql, committed test-first.
• Generated-file consistency: bash build/transform.sh at the merge commit reproduces sql/pgque.sql and sql/pgque-tle.sql byte-identically (empty git diff).

Filtered (candidates examined, excluded as non-issues)

• Security — security definer means the functions run as the owner (typically superuser). Safe here: static body, no dynamic SQL, pinned search_path, PUBLIC revoked; mirrors the existing get_consumer_info/get_batch_info pattern. Not a defect.
• Guidelines — the test file uses -- for a multi-line header where CLAUDE.md prefers /* ... */. Matches the established convention in sibling tests (e.g. test_e2e_role_split.sql), so it is consistent with the repo rather than a violation.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/pgque
number=267
target=github:NikolayS/pgque#267
state=MERGED
draft=false
diff_lines=157
diff_added=89
diff_removed=4
diff_bytes=5587
comments_count=3
commits_count=2
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)